Skip to content

Ribbon fuse pool - #1

Open
chudnov wants to merge 25 commits into
fuse-finalfrom
ribbon-fuse-pool
Open

chudnov wants to merge 25 commits into
fuse-finalfrom
ribbon-fuse-pool

Conversation

@chudnov

@chudnov chudnov commented Apr 1, 2022

Copy link
Copy Markdown

Tests can be found in governance repo pr. It includes same RibbonVaultAssetOracle.sol and simplified versions of CERC20.sol and RewardsDistributorDelegate.sol

@chudnov
chudnov changed the base branch from master to fuse-final April 1, 2022 21:31
import "./interfaces/ILiquidityGauge.sol";
import "./interfaces/IRibbonVault.sol";
import {DSMath} from "./libraries/DSMath.sol";

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests for this

* @param cToken The market whose COMP speed to update
*/
function _updateSpeedWithNewEpoch(CToken cToken) public {
require(block.timestamp.sub(startTime) >= WEEK, "Must be at least week since latest epoch");

@chudnov chudnov Apr 5, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

potential issues:

1: if _updateSpeedWithNewEpoch not called from cron job w fast enough cadence it could be spreading rewards for longer duration than the speed duration was set for (longer than a week). But should not be a problem as long as we continue minting each week so even if we call late they can claim from balance of subsequent (essentially kick can down road indefinitely)

2: if AVG_BLOCKS_PER_WEEK is off, we could be giving out more rewards than speed duration set for (ex: we assume AVG is x but we end up having 1.0025x blocks so we spread out same rewards for more blocks. But should average out and again should not be a problem as long as we continue minting each week so even if we call late they can claim from balance of subsequent (essentially kick can down road indefinitely).

/**
* @notice Set new borrow / supply speed
* @param cToken The market whose COMP speed to update
*/

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_updateSpeedWithNewEpoch needs to be called from cron job weekly

Comment thread contracts/CErc20.sol Outdated
// Minter contract for rbn gauge emissions
RibbonMinter public constant RBN_MINTER = RibbonMinter(0x5B0655F938A72052c46d2e94D206ccB6FF625A3A);
// RBN token
IERC20Upgradeable public constant RBN = IERC20Upgradeable(0x6123b0049f904d730db3c36a31167d9d4121fa6b);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use IERC20 because we're not touching any of the upgradeable functionality

Comment thread contracts/CErc20.sol
// Underlying is the gauge token like rETH-THETA-gauge
RBN_MINTER.mint(underlying)

uint256 toDistribute = RBN.balanceOf(address(this));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can add these lines to return early

if (toDistribute == 0) {
  return;
}

bool distributorAdded = false;
address[] memory distributors = comptroller.getRewardsDistributors();
for (uint256 i = 0; i < distributors.length; i++) if (distributors[i] == address(this)) distributorAdded = true;
for (uint256 i = 0; i < distributors.length; i++) if (distributors[i] == address(this)) distributorAdded = true;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gas optimization for loop:

uint256 distributorsLen = distributors.length;
for (uint256 i = 0; i < distributorsLen; ++i) 

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is distributors.length really calculated multiple times? this code was not changed its from original comp

* @notice Set borrower PCT
*/
function _setBorrowerPCT(uint256 _borrowerPCT) public {
require(msg.sender == admin, "only admin can set comp speed");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit only admin can set borrower percent

* @notice Set new borrow / supply speed
* @param cToken The market whose COMP speed to update
*/
function _updateSpeedWithNewEpoch(CToken cToken) public {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can rename this to be without underscore since it's public updateSpeedWithNewEpoch. We can also change it to external

function updateSpeedWithNewEpoch(CToken cToken) external {

* @notice Burn
* @param Takes in RBN tokens
*/
function burn(uint256 amount) public {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can change this to external

Comment thread contracts/CErc20.sol Outdated

uint256 toDistribute = RBN.balanceOf(address(this));

RBN.approve(rewardsDistributor, toDistribute)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't want to do an infinite approval once, like in becomeImplementationData?


IRibbonVault vault = IRibbonVault(ILiquidityGauge(underlying).lp_token());
uint256 rVaultDecimals = vault.decimals();
uint256 rVaultToAssetExchangeRate = vault.pricePerShare(); // (ex: rETH-THETA -> ETH, rBTC-THETA -> BTC)

@kenchangh kenchangh Apr 11, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not have to lookup the Uniswap v3 pool for this price?

Comment thread contracts/CErc20.sol
@@ -1,18 +1,35 @@
pragma solidity ^0.5.16;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests for this


/// @dev Notice that this contract is a RewardsDistributor
bool public constant isRewardsDistributor = true;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests for this

@chudnov
chudnov changed the base branch from fuse-final to fuse-reactive-audit May 16, 2022 22:18
@chudnov
chudnov changed the base branch from fuse-reactive-audit to fuse-final May 16, 2022 22:18
@chudnov
chudnov changed the base branch from fuse-final to fuse-reactive-audit May 16, 2022 22:18
@chudnov
chudnov changed the base branch from fuse-reactive-audit to fuse-final May 16, 2022 22:19
@chudnov
chudnov changed the base branch from fuse-final to fuse-reactive-audit May 16, 2022 22:36
@chudnov
chudnov changed the base branch from fuse-reactive-audit to fuse-final May 16, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants